Mark a shipment as canceled if all the inventory units have been canceled#5220
Mark a shipment as canceled if all the inventory units have been canceled#5220BenMorganIO wants to merge 2 commits intosolidusio:mainfrom
Conversation
github-actions
bot
added
the
changelog:solidus_core
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5220 +/- ##
==========================================
- Coverage 88.67% 88.66% -0.01%
==========================================
Files 564 564
Lines 13894 13898 +4
==========================================
+ Hits 12320 12323 +3
- Misses 1574 1575 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kennyadsl
changed the title
|
Thanks for working on this @BenMorganIO! I tried this locally with @MassimilianoLattanzio, who worked on something similar for one of our stores. We think this works, but we are not sure about if it's a good thing to consider a shipped shipment as canceled if all of its content has been canceled. E.g. I ship an empty package by mistake, so I will cancel all the items in it. Is it correct to consider that shipment canceled, even if it has been shipped? Yes, there's always the carton that represents the shipped shipment, but maybe in this case it's not clear what actually happened looking at the order's shipment state. What do you think? Also, I think there was another issue (already there, not related to this PR), because we are updating the shipment date of the shipment after canceling one of the items present in a shipped shipment, so in that case, the shipping date will represent the "all-items cancelation" date. |
|
If a shipped shipment has all of its items marked as cancelled, it should remain shipped since the shipped state is supposed to be immutable. |
|
But I don't see this happening with your code. Maybe I'm missing something? |
|
@BenMorganIO let me show that with a quick video (I'm using your branch): Screenshot.2023-07-11.at.10.19.42.mp4 |
|
@kennyadsl I can't understand why someone would cancel all of the items in a shipment after it has been shipped. I think a return would be started if that was the case. However, I want to keep the code so that once a shipment has been shipped, it is effectively immutable and should not transition to |
|
|
||
| shipments.each do |shipment| | ||
| if shipment.inventory_units.all? { |iu| iu.shipped? || iu.canceled? } | ||
| if shipment.inventory_units_canceled? |
There was a problem hiding this comment.
@kennyadsl I likely need to add a check here if the shipment is not in a shipped state.
There was a problem hiding this comment.
I'm having a look at the state machine of the shipment and it seems like it's not allowed to move a shipment from shipped to canceled already:
Do you know if there's a specific reason not to rely on the state machine here? Same for when we set the state to shipped. I think we are also skipping the rest of the callbacks defined in the state machine.
There was a problem hiding this comment.
No, I should be able to use the state machine call. I was mimicking the original code and thought that there must have been a reason to jump the state machine. Although, we should likely use it.
For the code on line 124, should we update that code as well to utilize the state machine?
There was a problem hiding this comment.
Yes, I think the best path forward at this point is to make another PR that just changes OrderCancelations to try to use the state machine. If that works and we can't find blockers, changing this PR will be smooth. What do you think?
You are right; it's hard to find a use case that resonates with this and the most realistic thing I could think of is having two items in the package (e.g., two glasses), both broken. Probably, you don't want to make a return for broken items, and canceling them would work. Do you think it's something worth taking into account? |
|
@kennyadsl I think if we keep shipped shipments retained as shipped, then it shouldn't be an issue. I can see a customer returning all items from a shipment or an admin canceling the items if the shipment was lost or something. Either way, we do have a comment in the code that the shipped state is supposed to be immutable. |
|
@solidusio/core-team now that we merged and shipped #6314 can we close this? |
|
Should be fixed by #6314. If not please open a new issue |
3 participants
Summary
I noticed that if a multi-shipment order goes through and you cancel one of the shipments, an order recalculation will set the shipment as pending/ready. See:
https://github.com/solidusio/solidus/blob/main/core/app/models/spree/shipment.rb#L208
This is because it's only checking if the order is canceled. While this is a correct method to check if a shipment should be marked as canceled, it falls short if it's only 1 shipment in an order that needs to be canceled out of many. If I cancel a shipment, then the shipment will be marked as shipped and that can be quite surprising for customers and admins.
Instead, let's increase the guarantee on canceling a shipment for now by checking if the order is canceled or all of the inventory units for said shipment have been canceled.
I've also updated the logic for checking if the state should be set to
shipped. While the check forshipped?remains to ensure immutability, we also mark a shipment as shipped just like in theSpree::OrderCancellationsif some of the inventory units are shipped and some are canceled. See:https://github.com/solidusio/solidus/blob/main/core/app/models/spree/order_cancellations.rb#L121
In the future, I think it would be better that when a shipment is canceled, an inventory unit cancellation should be done on the inventory units it's responsible for. That would help create a guarantee for resolving the bug highlighted above but this is a much larger change. For now, just marking a shipment as canceled when the inventory units have been canceled and matching the existing order cancelation logic is progress in the right direction.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: